Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove allocation in parse_identifier #12103

Merged
merged 1 commit into from
Jun 29, 2024
Merged

Remove allocation in parse_identifier #12103

merged 1 commit into from
Jun 29, 2024

Conversation

MichaReiser
Copy link
Member

Summary

I think this PR removes an allocation from parse_identifier.

parse_identifier converts the Box<str> from the TokenValue::Name to a String by calling .to_string.
to_string is a small wrapper around format!("{name}") which allocates a new string without reusing the Box<str> allocation, unless the compiler can see through the operation.

This PR uses Box<str>::into_string which, to my knowledge, reuses the underlying allocation.

Test Plan

cargo test

@MichaReiser MichaReiser requested a review from dhruvmanila as a code owner June 29, 2024 12:55
@MichaReiser MichaReiser added performance Potential performance improvement parser Related to the parser labels Jun 29, 2024
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jun 29, 2024

CodSpeed Performance Report

Merging #12103 will improve performances by 7.44%

Comparing parse-identifier-reduce-alloc (83894c9) with main (47b2273)

Summary

⚡ 4 improvements
✅ 26 untouched benchmarks

Benchmarks breakdown

Benchmark main parse-identifier-reduce-alloc Change
⚡ parser[large/dataset.py] 5.4 ms 5 ms +7.44%
⚡ parser[numpy/ctypeslib.py] 1,007.9 µs 954.6 µs +5.59%
⚡ parser[pydantic/types.py] 2.2 ms 2.1 ms +5.9%
⚡ parser[unicode/pypinyin.py] 329.3 µs 315 µs +4.54%

@MichaReiser
Copy link
Member Author

I'll go ahead and merge this. This change should be pretty uncontroversial 😆

@MichaReiser MichaReiser merged commit da78de0 into main Jun 29, 2024
20 checks passed
@MichaReiser MichaReiser deleted the parse-identifier-reduce-alloc branch June 29, 2024 13:00
@github-actions GitHub Actions
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

demisto/content (error) 

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'ignore' -> 'lint.ignore'
  - 'select' -> 'lint.select'
  - 'unfixable' -> 'lint.unfixable'
  - 'per-file-ignores' -> 'lint.per-file-ignores'
warning: `PGH001` has been remapped to `S307`.
warning: `PGH002` has been remapped to `G010`.
warning: `PLR1701` has been remapped to `SIM101`.
ruff failed
  Cause: Selection of deprecated rule `E999` is not allowed when preview is enabled.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error) 

warning: Detected debug build without --no-cache.
error: Failed to parse examples/gpt_actions_library/.gpt_action_getting_started.ipynb:11:1:1: Expected an expression
error: Failed to parse examples/gpt_actions_library/gpt_action_bigquery.ipynb:13:1:1: Expected an expression

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 2 project errors)

demisto/content (error) 

ruff format --preview --exclude Packs/ThreatQ/Integrations/ThreatQ/ThreatQ.py

warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'ignore' -> 'lint.ignore'
  - 'select' -> 'lint.select'
  - 'unfixable' -> 'lint.unfixable'
  - 'per-file-ignores' -> 'lint.per-file-ignores'
warning: `PGH001` has been remapped to `S307`.
warning: `PGH002` has been remapped to `G010`.
warning: `PLR1701` has been remapped to `SIM101`.
ruff failed
  Cause: Selection of deprecated rule `E999` is not allowed when preview is enabled.

openai/openai-cookbook (error) 

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to parse examples/gpt_actions_library/.gpt_action_getting_started.ipynb:11:1:1: Expected an expression
error: Failed to parse examples/gpt_actions_library/gpt_action_bigquery.ipynb:13:1:1: Expected an expression

@MichaReiser MichaReiser mentioned this pull request Jun 29, 2024
Merged
@AlexWaygood AlexWaygood changed the title Remove allcation in parse_identifier Remove allocation in parse_identifier Jun 29, 2024
@BrewTestBot BrewTestBot mentioned this pull request Jul 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhruvmanila dhruvmanila Awaiting requested review from dhruvmanila dhruvmanila is a code owner

Assignees
No one assigned
Labels
parser Related to the parser performance Potential performance improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@MichaReiser